-
Notifications
You must be signed in to change notification settings - Fork 329
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use the Ruby files of the FFI gem on TruffleRuby >= 20.1.0 #768
Use the Ruby files of the FFI gem on TruffleRuby >= 20.1.0 #768
Conversation
99f1764
to
7c577eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I think we'll need TruffleRuby and JRuby in CI of this project.
7c577eb
to
16f221a
Compare
Regarding adding JRuby in CI I'll leave that to @headius or to you (it should be trivial to add it to |
lib/ffi.rb
Outdated
JRuby::Util.load_ext("org.jruby.ext.ffi.FFIService") | ||
require 'ffi/ffi' | ||
|
||
elsif RUBY_ENGINE == 'truffleruby' && Gem::Version.new(RUBY_ENGINE_VERSION) >= Gem::Version.new("20.1.0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could tweak this condition to >= Gem::Version.new("20.1.0-dev-a")
once the related changes are merged in TruffleRuby (nightly versions look like 20.1.0-dev-374d84d2
).
That way the CI running on truffleruby-head would return true
for this condition.
(Potentially we could use RUBY_RELEASE_DATE
too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done this, the latest TruffleRuby nightly now has the changes needed.
Along with updating the FFI .rb files, I also made sure all specs are green on JRuby. |
@headius Could you make a PR adding |
@eregon I just checked and there's a handful of failures in recent spec updates related to the fixnum/bignum/long threshold. I would prefer to wait on adding JRuby to CI until we can use the gem for all FFI ruby sources (i.e. delete our copies of libs and specs), so I know there's no conflict between FFI head and JRuby head. |
That wouldn't work with older versions of the FFI gem though, so I'm not sure how feasible is that. |
@larskanis Looks like it's working just fine with |
I don't know what you mean here. Why would I test against an older version of the gem? |
Users might install an older version of the gem on the future JRuby release without FFI Ruby files. That would then break. So I think we need to keep Ruby files also in Ruby implementations as long as we want to support older FFI releases. |
@eregon I think you misunderstand... we would still include FFI as a default gem, like most of the other standard libraries. Installing an older FFI gem would not install any new files, so the ones in stdlib would still be used. Newer FFI gems would be picked up and used. I don't see a problem. |
FWIW I don't really intend to "support" older FFI gems in JRuby because they're all stub gems with no content. |
Right, that should work if that version of the default gem is recent or includes the JRuby version check in ffi.rb. A default gem is not very different than a copy in stdlib + a .gemspec anyway. |
@larskanis Could you make a release of FFI? This PR will help us address an issue happening with Line 21 in 9e6892c
We could also try to adapt the code there to compare against expanded ( File.expand_path ) entries of $LOAD_PATH but I think that would be kind of slow and I think it's mostly a Bundler issue. Also, with this PR, TruffleRuby no longer uses that part of the code anyway, which is cleaner and avoids the "require gem but remove it and go to stdlib" part.
|
Isn't that exactly what a default gem is? |
Yeah, exactly so in the end whether there is a copy of the |
Similar logic as for JRuby.
This will only apply for TruffleRuby >= 20.1.0 (not yet released), as versions before don't expect that.
I ran it locally with this and changes in TruffleRuby and all specs passed.
Ref #660